I was considering factoring out ... Does that seem like a reasonable thing to do?

Yeah, it's probably a decent idea.

I have refactored the test suite to reduce the duplication.

I asked in Slack in #SIG-api-machinery and got confirmation that configmaps are preferrred over endpoints, however in 1.12 there will be a dedicated object coming for this purpose, so perhaps make a TODO for upgrading it once the controller-runtime gets upgraded to 1.12.

Where would you default the ID and the namespace? I imagined these would be defaulted by adding some flags to wherever the manager is initialised (one of the templates in controller-tools), mine looks like this at the moment:

	mgr, err := manager.New(cfg, manager.Options{
		LeaderElection:          *leaderElection,
		LeaderElectionID:        *leaderElectionID,
		LeaderElectionNamespace: *leaederElectionNamespace,
	})

Where those variables come from flags.

TODO for upgrading it once the controller-runtime gets upgraded to 1.12.

Yes, please add the TODO (// TODO(JoelSpeed): switch to leaderelection object in 1.12) and/or file an issue

Where would you default the ID and the namespace

Namespace should default to the current namespace, when we can figure out it (e.g. via some default downward API environment variable). ID should have some sane default assuming this is the only controller in the namespace, and then we can make it overridable if need be. We should endeavor to make it so that the defaults work when using the setups that we describe in our docs and in kubebuilder.

Yes, please add the TODO (// TODO(JoelSpeed): switch to leaderelection object in 1.12) and/or file an issue

Added.

Namespace should default to the current namespace

I've added a getInClusterNamespace that attempts to read the namespace from the service account credentials added to a Pod. This should work in most cases I believe. If people aren't mounting service account credentials for their controllers then this won't work however, I can't think when that would be the case though. It returns a "nice" error suggesting you should set the namespace when it isn't running in cluster.

ID should have some sane default assuming this is the only controller in the namespace

I've defaulted it to controller-runtime for now, is about the most sensible thing I could come up with.

I like the defaulting logic, I think this looks decent. Does the leader election ConfigMap get created with the ID as the name, verbatim? If so, perhaps something like "controller-leader-election-helper" for the ID.

🚲 🏠

Does the leader election ConfigMap get created with the ID as the name, verbatim?

Yes, precisely. If the ID is controller-runtime then the ConfigMap would be called controller-runtime, I'm happy to change it as per your suggestion, I'll make the change

There were merge conflicts so I've just rebased on top of master

I fixed the tests that were failing, I think this might be ready to go?

PTAL @DirectXMan12 @pwittrock

Yeah, sorry, trying to do one last scan-through before lgtm.

To be clear, when we lose the leader election lock, we exit out, and presumably the pod gets restarted? Can you add a note in the cm.start or Start logic about that? Otherwise, it's a bit confusing to reason through. Afterwards, good to go.

@DirectXMan12 I've added a note about the behaviour and what to expect when a leader loses leadership. Does that seem sufficient?

Perfect. Can you squash down into "logical" commits? We can merge after that.

@DirectXMan12 Squashed as per your request 😄

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here